Skip to content

Add session management during shutdown and improve session handling #915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
bboynton97 merged 2 commits into from
Apr 11, 2025
Merged

Add session management during shutdown and improve session handling #915

bboynton97 merged 2 commits into from
Apr 11, 2025

Conversation

@Dwij1704
Copy link
Member

@Dwij1704 Dwij1704 commented Apr 9, 2025

📥 Pull Request

📘 Description
Closes #914
Add session management during shutdown and improve session handling

  • Introduced a global atexit handler to automatically end active sessions on shutdown.
  • Updated the Client class to register the active session globally.
  • Enhanced the end_session function to clear the client active session reference appropriately.
  • Improved error handling during session ending to ensure robustness.
  • Added context initialization in telemetry setup for better validation.

@codecov
Copy link

codecov bot commented Apr 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 52.94118% with 40 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
agentops/legacy/__init__.py 52.27% 21 Missing ⚠️
agentops/client/client.py 40.00% 12 Missing ⚠️
agentops/sdk/decorators/utility.py 68.42% 6 Missing ⚠️
agentops/sdk/core.py 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Dwij1704 Dwij1704 linked an issue Apr 9, 2025 that may be closed by this pull request
agentops.init() Not Creating Session Span by Default; start_session() Creates Sibling Span Without Context Propagation #894
Closed
@Dwij1704 Dwij1704 requested a review from bboynton97 April 9, 2025 15:48
@bboynton97
Copy link
Contributor

bboynton97 commented Apr 9, 2025

image
  1. two session urls printed
  2. incorrect API key should not be an exception

@bboynton97
Copy link
Contributor

bboynton97 commented Apr 9, 2025

double print is actually expected. one at kickoff and the other at the end of the run. API key was an issue with local environment.

nice job @Dwij1704 :)

bboynton97
bboynton97 reviewed Apr 9, 2025
View reviewed changes
bboynton97
bboynton97 reviewed Apr 9, 2025
View reviewed changes
bboynton97
bboynton97 approved these changes Apr 9, 2025
View reviewed changes
Copy link
Contributor

@bboynton97 bboynton97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this definitively limits us to one trace at a time. meaning we cannot track multiple agents running at once with the same agentops client.

this may need to be refactored for that in the future if/when that functionality is required

@bboynton97 bboynton97 requested a review from tcdent April 9, 2025 23:20
@bboynton97
Copy link
Contributor

bboynton97 commented Apr 9, 2025

verified it works with manual testing connected to prod

@bboynton97
Copy link
Contributor

bboynton97 commented Apr 10, 2025

Is this ready to merge @Dwij1704 ?

@Dwij1704
Copy link
Member Author

Dwij1704 commented Apr 10, 2025

Is this ready to merge @Dwij1704 ?

Yes!

@bboynton97 bboynton97 merged commit e1c8562 into main Apr 11, 2025
7 of 10 checks passed
@bboynton97 bboynton97 deleted the fix-agentops.init()-session-span branch April 11, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bboynton97 bboynton97 bboynton97 approved these changes

@tcdent tcdent Awaiting requested review from tcdent

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

agentops.init() Not Creating Session Span by Default; start_session() Creates Sibling Span Without Context Propagation

3 participants

@Dwij1704 @bboynton97